Skip to content

Fix exception IP in JIT #13929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Fix exception IP in JIT #13929

merged 1 commit into from
Apr 10, 2024

Conversation

bwoebi
Copy link
Member

@bwoebi bwoebi commented Apr 10, 2024

Not sure why IP was being loaded the dereferenced value of EG(exception_op), being the ZEND_EXCEPTION_HANDLER itself rather than &EG(exception_op)[0].
I believe that was wrong. I'm surprised the code worked at all...

This fixes the current failures in nightly github actions too: https://github.com/php/php-src/actions/runs/8608918170/job/23592077166

Please verify @dstogov.

| mov IP, aword [IP + (struct.._offset + offsetof(zend_..struct, field))]
| lea IP, aword [IP + (struct.._offset + offsetof(zend_..struct, field))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix is right!

@@ -1884,6 +1884,9 @@ static int zend_jit_leave_throw_stub(dasm_State **Dst)
|5:
| // opline = EG(exception_op);
| LOAD_IP_ADDR_ZTS executor_globals, exception_op
|| if (ZEND_OBSERVER_ENABLED) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to remove the if (ZEND_OBSERVER_ENABLED) { condition.
We already have the corresponding unconditional STORE in master.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do :-)

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove if (ZEND_OBSERVER_ENABLED) { condition and merge this into PHP_8_2 and PHP_8_3.
Master already have the right code.

Thanks for catching this!

@bwoebi bwoebi force-pushed the fix-exception-ip branch from b88af78 to b22acaf Compare April 10, 2024 17:04
@bwoebi bwoebi merged commit ea927ca into php:PHP-8.2 Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants